perf: integer-only forward elimination for gauss_solve (#72)#96
Merged
acgetchell merged 3 commits intomainfrom Apr 21, 2026
Merged
perf: integer-only forward elimination for gauss_solve (#72)#96acgetchell merged 3 commits intomainfrom
acgetchell merged 3 commits intomainfrom
Conversation
Replace BigRational-only gauss_solve with a hybrid that runs
Bareiss fraction-free forward elimination in BigInt on the
augmented (A | b) system, then back-substitutes in BigRational.
Eliminates GCD normalization from the O(D^3) phase while keeping
rational overhead limited to the cheaper O(D^2) back-sub.
Scope f64_to_bigrational to cfg(test); production code paths now
use f64_decompose directly (shared with bareiss_det_int).
Closes #72
Co-Authored-By: Oz <oz-agent@warp.dev>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced the prior BigRational-based exact solve with a hybrid pipeline: IEEE‑754 f64 decomposition → scale to a common 2^e_min → Bareiss fraction-free forward elimination on Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Decomp as f64_decompose
participant FwdElim as Forward Elimination<br/>(BigInt Bareiss)
participant BackSub as Back-Substitution<br/>(BigRational)
participant Result as Solution
Caller->>Decomp: Decompose matrix & RHS (IEEE‑754 → mantissa × 2^exp)
Decomp-->>FwdElim: mantissas, exponents, e_min
FwdElim->>FwdElim: Scale to common 2^e_min → BigInt augmented matrix
FwdElim->>FwdElim: Bareiss fraction-free elimination (O(D³)), mirror RHS on row swaps
FwdElim-->>BackSub: Upper-triangular BigInt matrix + scaled RHS
BackSub->>BackSub: Convert needed BigInt entries → BigRational
BackSub->>BackSub: Rational back-substitution (O(D²))
BackSub-->>Result: Exact `BigRational` solution vector
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
- Coverage 94.74% 93.16% -1.59%
==========================================
Files 5 5
Lines 514 512 -2
==========================================
- Hits 487 477 -10
- Misses 27 35 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Migrate the exact linear solver to a hybrid approach that performs O(D³) forward elimination using fraction-free Bareiss updates on integers, limiting BigRational arithmetic to the O(D²) back-substitution phase. This eliminates per-entry GCD overhead during elimination. Internal f64 decomposition is unified via f64_decompose, and f64_to_bigrational is moved to test scope. Refs: #72
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/exact.rs (1)
1650-1709: Consider macro-generating these hybrid edge cases for D=2 through D=5.These are valuable regressions for the generic
solve_exactpath, but most are pinned to D=2/D=3. The diagonal, subnormal-RHS, and pivot-block cases can be embedded in larger identity systems so the same hybrid path is covered across D=2–5.Example shape for extending one case
+ macro_rules! gen_solve_exact_large_finite_entries_tests { + ($d:literal) => { + paste! { + #[test] + fn [<solve_exact_large_finite_entries_ $d d>]() { + let big = f64::MAX / 2.0; + assert!(big.is_finite()); + + let mut rows = [[0.0f64; $d]; $d]; + let mut b_arr = [0.0f64; $d]; + for i in 0..$d { + rows[i][i] = big; + b_arr[i] = if i + 1 == $d { 0.0 } else { big }; + } + + let a = Matrix::<$d>::from_rows(rows); + let b = Vector::<$d>::new(b_arr); + let x = a.solve_exact(b).unwrap(); + for i in 0..$d { + let expected = if i + 1 == $d { 0 } else { 1 }; + assert_eq!(x[i], BigRational::from_integer(BigInt::from(expected))); + } + } + } + }; + } + + gen_solve_exact_large_finite_entries_tests!(2); + gen_solve_exact_large_finite_entries_tests!(3); + gen_solve_exact_large_finite_entries_tests!(4); + gen_solve_exact_large_finite_entries_tests!(5);As per coding guidelines, tests for dimension-generic code must cover D=2 through D=5 whenever possible using macros for per-dimension test generation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exact.rs` around lines 1650 - 1709, Tests currently only cover specific dimensions (D=2/3); create macro-generated tests for D=2..=5 to satisfy the guideline. Add a macro (using macro_rules!) that expands each existing test case (solve_exact_large_finite_entries, solve_exact_mixed_magnitude_entries, solve_exact_subnormal_rhs, solve_exact_pivot_swap_with_fractional_result) for dimensions 2 through 5 by constructing matrices/vectors of generic size: for diagonal/large/mixed/subnormal cases build a D×D diagonal or identity padded matrix using Matrix::<D>::from_rows and Vector::<D>::new; for the pivot swap case embed the 2×2 test block into the top-left of a D×D identity and pad RHS accordingly so the same expected BigRational results hold in the corresponding indices; generate distinct test function names per D (e.g. solve_exact_large_finite_entries_D3) so each expands to a #[test] and uses the same assertions (adjusting indices where embedded).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/exact.rs`:
- Around line 1650-1709: Tests currently only cover specific dimensions (D=2/3);
create macro-generated tests for D=2..=5 to satisfy the guideline. Add a macro
(using macro_rules!) that expands each existing test case
(solve_exact_large_finite_entries, solve_exact_mixed_magnitude_entries,
solve_exact_subnormal_rhs, solve_exact_pivot_swap_with_fractional_result) for
dimensions 2 through 5 by constructing matrices/vectors of generic size: for
diagonal/large/mixed/subnormal cases build a D×D diagonal or identity padded
matrix using Matrix::<D>::from_rows and Vector::<D>::new; for the pivot swap
case embed the 2×2 test block into the top-left of a D×D identity and pad RHS
accordingly so the same expected BigRational results hold in the corresponding
indices; generate distinct test function names per D (e.g.
solve_exact_large_finite_entries_D3) so each expands to a #[test] and uses the
same assertions (adjusting indices where embedded).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31d45828-074e-4e3e-ad5e-4e76d786502f
📒 Files selected for processing (2)
REFERENCES.mdsrc/exact.rs
Major refactor of `src/exact.rs`:
- Extract shared Bareiss primitives (`decompose_matrix`, `decompose_vec`,
`component_to_bigint`, `build_bigint_matrix`, `build_bigint_vec`,
`bareiss_forward_eliminate`) so `bareiss_det_int` and `gauss_solve`
share a single integer-Bareiss core.
- Hybrid BigInt/BigRational solve: forward elimination runs entirely in
`BigInt` with fraction-free Bareiss updates on `(A | b)`; only the
`O(D²)` back-substitution phase lifts into `BigRational`.
- `mem::take` in back-substitution eliminates per-entry `clone()` calls
on `rhs[i]`, `a[i][j]`, `a[i][i]`.
Structured errors replace preconditions:
- `decompose_matrix` / `decompose_vec` fold `is_finite()` into the same
pass that decomposes each entry and return
`Err(LaError::NonFinite { row, col })` on the first non-finite cell.
- `bareiss_det_int`, `bareiss_det`, `gauss_solve` now return
`Result<_, LaError>`; error propagates to every public entry point via
`?`.
- `validate_finite` and `validate_finite_vec` removed (dead after the
refactor); `det_sign_exact` relies on IEEE 754 NaN/∞ propagation
through `det_direct()` plus `bareiss_det_int` for Stage-2 validation.
Polish:
- Promote `Component` from tuple alias to `#[derive(Clone, Copy, Default)]`
struct with named `mantissa`/`exponent`/`is_negative` fields.
- `BareissResult` derives `Debug`.
- Debug-only post-conditions in `bareiss_forward_eliminate` assert
upper-triangular shape + non-zero pivots (zero cost in release).
- `cold_path()` hints at every rare branch (singular detection inside
`bareiss_forward_eliminate`, singular match arm in `bareiss_det_int`,
non-finite detection in `decompose_*`).
- Hoist `core::mem::take` import to module top; use unqualified `take`.
- Refresh module-level docs with a `## Validation` section describing
the single-pass validate+decompose flow.
Tests:
- Macro-generated D=2..5 coverage for `solve_exact`:
`large_finite_entries`, `mixed_magnitude_entries`, `subnormal_rhs`,
`pivot_swap_with_fractional_result`, `mid_pivot_swap` (D=3..5),
`singular_rank_deficient`, `zero_rhs`.
- Direct `Err(LaError::NonFinite)` assertions for `bareiss_det_int`
(NaN/∞ input).
- Consolidate four `bareiss_det_int_d1_*` tests into one table-driven
`bareiss_det_int_d1_cases`.
- Remove three `validate_finite_*` and three `validate_finite_vec_*`
tests whose coverage is now transitively exercised via the public API
error-path tests.
Benchmarks (`cargo bench --features "bench exact" --bench exact`):
- exact_d3/solve_exact: -42% to -43%
- exact_d4/solve_exact: -53% to -54%
- exact_d3/solve_exact_f64: -42% to -43%
- exact_d4/solve_exact_f64: -54% to -55%
- exact_d3/det_sign_exact: -26% to -27%
- exact_d4/det_sign_exact: -15% to -16%
- exact_d4/det_exact(_f64): +2% to +4% (minor; Result propagation)
- exact_d5: no change detected
Verification:
- `cargo test --features exact --lib`: 359 passed, 0 failed.
- `cargo clippy --features exact --all-targets -- -D warnings`: clean.
- `just ci`: passes.
Co-Authored-By: Oz <oz-agent@warp.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace BigRational-only gauss_solve with a hybrid that runs
Bareiss fraction-free forward elimination in BigInt on the
augmented (A | b) system, then back-substitutes in BigRational.
Eliminates GCD normalization from the O(D^3) phase while keeping
rational overhead limited to the cheaper O(D^2) back-sub.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests